-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-implement V3 Service Bindings #1158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took my first pass through this, and it looks good. There are no integration tests though. I'd like to see some basic coverage of the functionality in integration tests. It's one thing to test with mock responses, but we want to have test coverage that interacts with an actual CF environment too.
I know writing and running integration tests is not trivial. If you are able to put the tests together, I can run them in one of our labs.
Hi @dmikusa-pivotal , |
@radito3 I ran the integration tests and am getting an error, looks like the same one, across all the tests...
Still looking at the PR and playing with it, but was curious why override deserialize here. Did you hit a specific issue there, was the default implementation not deserializing the payload properly? Thanks |
Also, from what I can tell, it looks like CAPI 1.109.0, which is what we have in Tanzu Application Server 2.11 does contain the API for service binding credentials. I would suggest marking the test as |
Hi @dmikusa-pivotal, thanks for pointing out the errors, I had 2 small issues. They are fixed now. |
Excellent, thanks. Tests are passing, merging. |
No description provided.